Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: bounce notification #4107

Merged
merged 4 commits into from
Jul 8, 2022
Merged

fix: bounce notification #4107

merged 4 commits into from
Jul 8, 2022
+84 −12

Conversation

wanlingt
Copy link
Contributor

@wanlingt wanlingt commented Jul 6, 2022 •
edited
Loading

Problem

Users are spammed with multiple smses and emails after email-mode responses bounce

Closes #4094

Solution

Re-introduce hasNotified, which checks if form admins and collaborators have already been sms-ed or emailed after an email response's critical bounce.

Breaking Changes

  • No - this PR is backwards compatible

Tests

Same tests as those in Bounce module refactor PR

  • Create an email mode form. Add a valid collaborator with a registered contact number, and add a non-existent email as one of the email recipients. Submit the form. The form should NOT be deactivated, and the valid email recipient should not receive any emails/SMSes. Check the document in the Bounces collection corresponding to the form ID. The bounces array should contain the correct hasBounced key for each email address.
  • Remove all valid email recipients and leave only non-existent emails. Submit the form. The form should be deactivated, and the form admin + collaborators should each receive only ONE email and TWO smses, informing them of the bounce and form deactivation.

@wanlingt wanlingt temporarily deployed to staging-al2 July 6, 2022 07:09 Inactive
@wanlingt wanlingt marked this pull request as ready for review July 6, 2022 08:10
timotheeg
timotheeg reviewed Jul 7, 2022
View reviewed changes
Copy link
Contributor

@timotheeg timotheeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make reveiwing the changeset easier, load it with ignore-whitespace enabled.

Since the PR introduces a branching behaviour, a new test needs to be added where hasNotified() is set to return true, and the test verifies that sendEmailBounceNotification() is not called.

@wanlingt
Copy link
Contributor Author

wanlingt commented Jul 7, 2022 •
edited
Loading

Just to make reveiwing the changeset easier, load it with ignore-whitespace enabled.

Since the PR introduces a branching behaviour, a new test needs to be added where hasNotified() is set to return true, and the test verifies that sendEmailBounceNotification() is not called.

Thanks Tim! Didn't know about the whitespace setting, its really useful. I've added new tests for hasNotified() set to true

@wanlingt wanlingt requested a review from timotheeg July 8, 2022 02:09
timotheeg
timotheeg approved these changes Jul 8, 2022
View reviewed changes
Copy link
Contributor

@timotheeg timotheeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 👍

I think the unit tests could be refactored at some point to be a bit more granular, but we can revisit that another time. Let's get this out to help admins.

@wanlingt wanlingt merged commit 40c4489 into develop Jul 8, 2022
@wanlingt wanlingt deleted the fix/bounce-notification branch July 8, 2022 05:21
@justynoh justynoh mentioned this pull request Jul 12, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timotheeg timotheeg timotheeg approved these changes

@mantariksh mantariksh Awaiting requested review from mantariksh

@justynoh justynoh Awaiting requested review from justynoh

@karrui karrui Awaiting requested review from karrui

@tshuli tshuli Awaiting requested review from tshuli

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Users get spammed with bounce notifications when mail services are down
2 participants
@wanlingt @timotheeg